Potential fix for code scanning alert no. 6: Unvalidated dynamic method call#21
Potential fix for code scanning alert no. 6: Unvalidated dynamic method call#21ialejandro merged 2 commits intomainfrom
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Iván Alejandro Marugán <hello@ialejandro.rocks>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return handler(currentCode); | ||
| } | ||
| if (handlerToExecute) { | ||
| return handlerToExecute(currentCode); |
Check failure
Code scanning / CodeQL
Unvalidated dynamic method call High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
In general, to fix this kind of issue you must ensure that user-controlled strings cannot arbitrarily select or invoke unexpected methods. This is typically done by (a) restricting dispatch to a known whitelist of commands, (b) verifying that the selected value is an own property and a function, and (c) avoiding overly permissive matching logic that might invoke commands on partial matches in surprising ways. It’s also good practice to handle exceptions from invoked handlers so one bad handler or input cannot take down the whole endpoint.
For this specific code, the essential parts are already in place: an own-property check and a typeof === "function" check. The main remaining concerns are: (1) the looser prefix-matching path, and (2) the lack of a safety net around the handler call. To keep current behavior while improving safety, we can:
- Keep the exact-match logic as is (it already validates own property and function type).
- Keep the prefix-matching logic but explicitly ensure we’re only iterating over own properties (using
Object.entriesis fine) and already checkingtypeof handler === "function"; that’s good. - Add a defensive guard before invocation to verify
typeof handlerToExecute === "function"right at the call site; this guards against future changes wherehandlerToExecutemight accidentally be set to a non-function. - Wrap the invocation in a
try/catchblock so unexpected exceptions from handler code cannot crash the API; instead, return a controlled error message and non-zero exit code. This directly mitigates the DoS angle CodeQL is concerned about, without changing which commands are permitted.
All necessary changes are in src/lib/terminal/simulator.ts, around the final check and invocation of handlerToExecute. No import changes or new helper functions are required.
| @@ -48,8 +48,15 @@ | ||
| } | ||
| } | ||
|
|
||
| if (handlerToExecute) { | ||
| return handlerToExecute(currentCode); | ||
| if (typeof handlerToExecute === "function") { | ||
| try { | ||
| return handlerToExecute(currentCode); | ||
| } catch (error) { | ||
| return { | ||
| output: `Error: command execution failed`, | ||
| exitCode: 1, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Built-in commands |
|
🎉 This PR is included in version 1.1.2 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Potential fix for https://github.com/devops-ia/self-learning-platform/security/code-scanning/6
General approach: Keep dynamic command handling but ensure that any function we call is (a) associated with an allowed command string, (b) an own property of the commands map (not from the prototype chain), and (c) actually a function. Also avoid calling handlers multiple times or without consistent validation.
Best concrete fix while preserving behavior:
hasOwnProperty+typeof === "function"checks for exact matches.Object.entries, but to address the CodeQL concern and make the flow clearer, we can reuse the same validation pattern as for the exact match.exactHandlerorprefixHandler), then call it once. This keeps behavior the same but centralizes validation and the call site, making the sink easier to reason about.Concretely in
src/lib/terminal/simulator.ts:let handlerToExecute: ((code: string) => TerminalResponse) | undefinedand populate it either from the exact match or from the prefix loop, only when the value is a function.if (handlerToExecute)and call it exactly once.typeof handler !== "function"andObject.entries(which yields own enumerable properties), so the main behavioral change is to assign the chosen handler tohandlerToExecuteinstead of invoking it directly; this helps keep all invocations going through a single, validated call site.No changes are needed in
src/app/api/terminal/route.ts.Suggested fixes powered by Copilot Autofix. Review carefully before merging.